Skip to content

url: preprocess host/hostname setter input so IDNA matches the constructor#33757

Open
robobun wants to merge 3 commits into
mainfrom
farm/45ccfa6a/url-host-setter-idna-preprocess
Open

url: preprocess host/hostname setter input so IDNA matches the constructor#33757
robobun wants to merge 3 commits into
mainfrom
farm/45ccfa6a/url-host-setter-idna-preprocess

Conversation

@robobun

@robobun robobun commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

The URL host/hostname setter must run the basic URL parser with state override (https://url.spec.whatwg.org/#dom-url-host), which strips ASCII tab/newline from the input and then has the host parser UTF-8 percent-decode before domain-to-ASCII. WTF::URL::setHost instead hands any non-ASCII value straight to uidna_nameToASCII (via appendEncodedHostname in WTF/wtf/URL.cpp), so the tab/newline or the literal %xx is baked into the Punycode A-label and the setter disagrees with the constructor on the same input.

const set  = v => { const u = new URL("http://base/"); u.hostname = v; return u.hostname; };
const ctor = v => new URL(`http://${v}/`).hostname;

set("\tß.de");      // bun: "xn---qfa.de" (A-label decodes to "\tß")   node/ctor: "xn--zca.de"
set("ü\t.de");      // bun: "xn---dha.de"                              node/ctor: "xn--tda.de"
set("ü%41.de");     // bun: "xn--a-goa.de" (literal "%41" in the label) node/ctor: "xn--a-dha.de"
set("%C3%BCü.de");  // bun: "base" (silently dropped)                  node/ctor: "xn--tdaa.de"

set("a\tb.com");    // bun: "ab.com", already correct: the pure-ASCII path
                    // in appendEncodedHostname passes through and the full parser runs

The first two inputs mint hostnames whose A-label decodes to an embedded control character, which no spec-conforming host parser can produce (the forbidden-host-code-point check runs on the already-encoded label and cannot see it), so a filter that validates new URL(x) and then assigns u.hostname = x validates a different host than it uses.

Fix

appendEncodedHostname only diverges from the spec on non-ASCII input: for all-ASCII it appends the value as-is and the full URLParser then does the right thing on reparse. URLDecomposition::setHost/setHostname now strip ASCII tab/newline and UTF-8 percent-encode any non-ASCII code points before calling WTF::URL::setHost, which forces that ASCII passthrough and lets URLParser::parseHostAndPort do the spec's percent-decode + domain-to-ASCII exactly as the constructor does. For all-ASCII values with no tab/newline (the common case) the preprocessor is a no-op.

The upstream appendEncodedHostname itself lives in the prebuilt WebKit, so it is not touched here; the WHATWG setter semantics are owned by URLDecomposition in this repo.

Verification

11 new cases in test/js/web/url/url.test.ts assert setter == constructor for tab/LF/CR + non-ASCII, %xx + non-ASCII, mixed, surrogate-pair code points, the host setter with a port, and a pure tab/newline value. Every expected value matches Node 26. 10 of the 11 fail on the released Bun (the one pass is the pure-ASCII control case); all 25 tests in the file pass with this change.

test/js/node/url/url.test.ts and test/js/web/urlpattern/urlpattern.test.ts (414 tests) pass with the fix.

Relationship to open URLDecomposition PRs

#32826 (host/port split and origin), #33195 (empty-host guard), and #33206 (domainToASCII) touch the same file for adjacent bugs but none of them fixes this one: with each of those applied alone, u.hostname = "\tß.de" still yields "xn---qfa.de". #33206's PR body notes this exact mechanism ("setHost applies IDNA to its raw argument before the parse removes them") but only works around it inside domainToASCII. These changes conflict textually on URLDecomposition.cpp and url.test.ts; whichever lands second gets a small rebase.

The URL host/hostname setter must run the basic URL parser with state
override, which strips ASCII tab/newline and percent-decodes before
domain-to-ASCII. WTF::URL::setHost instead runs uidna_nameToASCII on the
raw value whenever it contains non-ASCII, so a tab or a percent-escape
is baked into the Punycode A-label and the setter disagrees with the
constructor on the same input.

Strip ASCII tab/newline and UTF-8 percent-encode non-ASCII in
URLDecomposition before calling WTF::URL::setHost so it takes its
ASCII passthrough and the full URLParser runs on reparse.
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 18 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b1987d9d-ec0f-46de-ba23-05bae285906d

📥 Commits

Reviewing files that changed from the base of the PR and between 332f744 and 4e87493.

📒 Files selected for processing (2)
  • src/jsc/bindings/URLDecomposition.cpp
  • test/js/web/url/url.test.ts

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the claude label Jul 8, 2026
@robobun

robobun commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 9:48 AM PT - Jul 8th, 2026

@robobun, your commit 4e874939837375c8c58e0defb954cc714d563e27 passed in Build #70498! 🎉


🧪   To try this PR locally:

bunx bun-pr 33757

That installs a local version of the PR into your bun-33757 executable, so you can run:

bun-33757 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find bugs, but this is a non-trivial workaround for WebKit's appendEncodedHostname that relies on forcing its ASCII-passthrough path, touches security-adjacent hostname parsing, and textually conflicts with three other open PRs on the same file — worth a human deciding on approach and merge order.

Extended reasoning...

Overview

Adds preprocessHostSetterValue() in src/jsc/bindings/URLDecomposition.cpp and calls it from both setHost and setHostname. The helper strips ASCII tab/LF/CR and UTF-8 percent-encodes any non-ASCII code points, so the value handed to WTF::URL::setHost is always pure ASCII. That forces WebKit's appendEncodedHostname onto its ASCII passthrough branch, after which the full URLParser reparse performs the spec's percent-decode + domain-to-ASCII correctly. 11 new test cases in test/js/web/url/url.test.ts assert setter output matches the constructor and Node.

Security risks

URL hostname parsing is security-adjacent — the PR description itself frames the bug as a filter-bypass primitive (validate new URL(x) then assign u.hostname = x and get a different host). The fix narrows that gap, which is good, but changes to host canonicalization deserve human eyes on edge cases (IPv6 literals, opaque hosts, non-special schemes, interaction with the port-splitting logic in setHost which now runs on the preprocessed string).

Level of scrutiny

Medium-high. The mechanism is a deliberate workaround coupled to internal WebKit behavior ("all-ASCII input takes the passthrough and reparses") rather than a direct fix, because the buggy code lives in the prebuilt WebKit. That's a reasonable pragmatic choice, but it's exactly the kind of design decision ("patch here vs. patch the fork") a maintainer should sign off on. The String preprocessed lifetime keeping the reassigned StringView value valid is correct but subtle.

Other factors

  • The PR body explicitly calls out textual conflicts with #32826, #33195, and #33206 on the same file; a human should decide merge order.
  • U8_APPEND_UNSAFE is justified by the IDLUSVString comment, which is correct for the WHATWG URL interface, but I did not exhaustively verify every caller of URLDecomposition::setHost guarantees USVString conversion.
  • Test coverage is good (tab/LF/CR × non-ASCII, percent-escapes, astral plane, port, empty-after-strip) and asserts against both the constructor and hard-coded Node-verified values.

@robobun

robobun commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

On the U8_APPEND_UNSAFE safety point: DOMURL is the only URLDecomposition subclass in Bun, and its only setHost/setHostname call sites are JSDOMURL.cpp:441-444 and :473-476, both of which go through convert<IDLUSVString>. URLPatternCanonical.cpp:129 calls WTF::URL::setHost on a plain URL, not URLDecomposition::setHost, so it does not reach this code.

@robobun

robobun commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

CI status: the diff itself is green. 284 of 287 jobs passed on build 70498 with no url-related failures; the only red is darwin 14 aarch64 - test-bun which expired waiting for an agent (the two scheduled retries are in the same queue). Build 70494 before it failed only on postgres-invalid-message-length.test.ts (Windows, ERR_POSTGRES_CONNECTION_REFUSED) plus known-flaky install and bake tests, none touching URL. Ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant